CNTRLPLANE-3323: Update openshift-hack/rebase.sh#2701
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@JSampsonIV: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesRebase flow and documentation updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@JSampsonIV: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
JSampsonIV
changed the title
|
@JSampsonIV: This pull request references CNTRLPLANE-3323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-hack/rebase.sh`:
- Line 23: The help text for the --jira-id option in the rebase.sh script is
misleading because it says it creates a Jira ticket, but the actual flow only
creates a PR with the Jira ID included in the title/body. Update the usage/help
message in the argument parsing or help output around the --jira-id description
to accurately state that it creates a pull request and not a Jira ticket,
keeping the wording aligned with the behavior implemented in the rebase flow.
- Line 101: The git checkout call in rebase.sh is malformed because -b is
consuming --track as the branch name. Update the checkout invocation around the
branch creation logic to either use git checkout --track
"openshift/$openshift_release" or place --track after the new branch name in the
git checkout -b path, so the branch is created/tracked correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4851948b-257c-4a28-996f-5ab1e68f501d
📒 Files selected for processing (1)
openshift-hack/rebase.sh
| git pull openshift "$openshift_release" | ||
|
|
||
| git merge "$k8s_tag" | ||
| # shellcheck disable=SC2181 |
| # openshift-hack/images/hyperkube/Dockerfile.rhel still has FROM pointing to old tag | ||
| # we need to remove the prefix "v" from the $k8s_tag to stay compatible | ||
| sed -i -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel | ||
| sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak |
There was a problem hiding this comment.
Is this a misplaced find/replace? -i.bak. Why are we deleting a .bak version of the Dockerfile?
There was a problem hiding this comment.
This was to allow the script to be run on MacOS, since it requires an extension argument for -i. It's then deleted as part of cleanup.
There was a problem hiding this comment.
Hmmm, maybe instead we could just run this in a container, much like the go mod tidy and friends; wouldn't necessarily have to be in the openshift rhel, could be something small, like the alpine docker image.
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@JSampsonIV: This pull request references CNTRLPLANE-3323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@JSampsonIV: This pull request references CNTRLPLANE-3323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift-hack/rebase.sh (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused
go_mod_go_verassignment.It’s no longer used after deriving
tagfrom the DockerfileFROMline.Suggested cleanup
-go_mod_go_ver=$(grep -E 'go 1\.[1-9][0-9]?' go.mod | sed -E 's/go (1\.[1-9][0-9]?)/\1/' | cut -d '.' -f 1,2) # Need to handle mod versions like 1.23 and 1.23.4; our release images only have major.minor tag=$(grep "^FROM" openshift-hack/images/hyperkube/Dockerfile.rhel | head -n1 | sed -E 's/.*:([^ ]+).*/\1/')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-hack/rebase.sh` at line 129, The `go_mod_go_ver` assignment in `rebase.sh` is now unused after `tag` is derived from the Dockerfile `FROM` line. Remove that leftover shell variable initialization and any related dead logic, keeping the surrounding tag derivation flow intact.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-hack/rebase.sh`:
- Line 128: The version-matching regex in the rebase.sh replacement for the
Dockerfile update is too restrictive and can miss valid Kubernetes versions with
0 components, so the substitution may not run. Update the sed expression used in
this step to match general semantic version strings in the openshift build
versions value, and keep the replacement behavior in the same rebase.sh block
that edits openshift-hack/images/hyperkube/Dockerfile.rhel.
---
Nitpick comments:
In `@openshift-hack/rebase.sh`:
- Line 129: The `go_mod_go_ver` assignment in `rebase.sh` is now unused after
`tag` is derived from the Dockerfile `FROM` line. Remove that leftover shell
variable initialization and any related dead logic, keeping the surrounding tag
derivation flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ceadba2c-cb99-4b4d-81cf-8f600ac5707d
📒 Files selected for processing (1)
openshift-hack/rebase.sh
| # openshift-hack/images/hyperkube/Dockerfile.rhel still has FROM pointing to old tag | ||
| # we need to remove the prefix "v" from the $k8s_tag to stay compatible | ||
| sed -i -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel | ||
| sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Line 128 regex can fail for valid versions containing 0.
(1.[1-9]+.[1-9]+) won’t reliably match values like 1.30.0, so the Dockerfile replacement may silently not happen.
Suggested fix
-sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak
+sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1\.[0-9]+\.[0-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1.[1-9]+.[1-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak | |
| sed -i.bak -E "s/(io.openshift.build.versions=\"kubernetes=)(1\.[0-9]+\.[0-9]+)/\1${k8s_tag:1}/" openshift-hack/images/hyperkube/Dockerfile.rhel && rm -f openshift-hack/images/hyperkube/Dockerfile.rhel.bak |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift-hack/rebase.sh` at line 128, The version-matching regex in the
rebase.sh replacement for the Dockerfile update is too restrictive and can miss
valid Kubernetes versions with 0 components, so the substitution may not run.
Update the sed expression used in this step to match general semantic version
strings in the openshift build versions value, and keep the replacement behavior
in the same rebase.sh block that edits
openshift-hack/images/hyperkube/Dockerfile.rhel.
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@JSampsonIV: This pull request references CNTRLPLANE-3323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift-hack/rebase.sh`:
- Around line 14-23: The CLI flag rename is not reflected in the contributor
docs, so the documented `openshift-hack/rebase.sh` command still uses the
removed `--bugzilla-id` option. Update `REBASE.openshift.md` to replace every
`--bugzilla-id` reference with `--jira-id`, keeping the example and surrounding
text aligned with the current `usage()` output in `openshift-hack/rebase.sh`.
Make sure any other mention of the old flag in the same docs section is renamed
consistently.
- Line 128: The in-place edit in the rebase script is using a BSD-only sed form,
which breaks on GNU sed in Linux/CI. Update the command in rebase.sh to use a
portable in-place edit approach (for example, a backup-suffix form with cleanup)
while keeping the existing replacement logic for the hyperkube Dockerfile.rhel
update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b34bd4d-f372-4666-bd7f-83406b1f5449
📒 Files selected for processing (1)
openshift-hack/rebase.sh
There was a problem hiding this comment.
I realize you didn't add this in this PR but just looking through this script for the first time... I think we have an issue here @dusk125. Isn't this && going to run go mod tidy in the container but the update-vendor script on the host? And then if so the return value check afterwards is looking at the wrong thing.
There was a problem hiding this comment.
Oh wow.... Yeah I think you're right... that could explain some issues I had had in the past!
You'd have to do /bin/bash -c "go mod tidy && hack/update-vendor.sh"
There was a problem hiding this comment.
Would that also explain the need for the additional commit in #2693?
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@JSampsonIV: This pull request references CNTRLPLANE-3323 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openshift-hack/rebase.sh (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused
go_mod_go_verassignment (Line 129).It is currently dead code after tag derivation moved to the Dockerfile
FROMline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift-hack/rebase.sh` at line 129, The go_mod_go_ver assignment in rebase.sh is dead code now that tag derivation moved to the Dockerfile FROM line. Remove the unused go_mod_go_ver variable setup and any related shell plumbing in the rebase logic, keeping the rest of the version/tag handling intact.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@REBASE.openshift.md`:
- Around line 525-527: The fenced shell command example in the rebase
documentation is missing a language tag, which will trigger markdown linting.
Update the code block around the openshift-hack/rebase.sh example to use a shell
label such as bash so the fenced block is properly annotated and remains
lint-clean.
---
Nitpick comments:
In `@openshift-hack/rebase.sh`:
- Line 129: The go_mod_go_ver assignment in rebase.sh is dead code now that tag
derivation moved to the Dockerfile FROM line. Remove the unused go_mod_go_ver
variable setup and any related shell plumbing in the rebase logic, keeping the
rest of the version/tag handling intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed6867cd-7e89-4d76-ae84-4ac61f38eef8
📒 Files selected for processing (2)
REBASE.openshift.mdopenshift-hack/rebase.sh
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@dusk125: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test verify |
openshift-ci-robot
removed
the
verified
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@dusk125 I had to push a change for the variable expansion on line 104, could you re-verify please |
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/retest |
|
@JSampsonIV: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
@jubittajohn can we just override these last failing tests since this is just a tooling change? |
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@dusk125: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dusk125, jacobsee, JSampsonIV The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold for the rebase to complete #2653 |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test all |
|
@JSampsonIV: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test all |
6 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Summary by CodeRabbit
--jira-id.